Repository navigation
Conversation
kfaraz
approved these changes
Oct 8, 2026
Contributor
There was a problem hiding this comment.
Thanks, @suraj-goel !
Before merging off this PR, could you also confirm if this change has affected the runtime of this test in anyway? Just curious.
Edit: never mind, read the PR description. It seems like the test will now finish slightly faster?
Contributor
Author
Thanks @kfaraz! Yes, it's faster. Measured locally:
|
Contributor
Author
|
@kfaraz , I don't have permissions to merge. Feel free to merge this PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #20518.
Description
EmbeddedKafkaSupervisorTest.test_runKafkaSupervisorWithHeaderFiltering(added in #18525) failed its first attempt in 6 of 7 master runs. Each time, it timed out waiting foringest/handoff/countto reach 4. It fails only when it runs after the other tests in the class. The surefire retry runs it alone, so the retry passed.Root cause
The task does read all 10 records. The race is between the test suspending the supervisor and the task's own checkpoint:
maxRowsPerSegment = 1, the task requests a checkpoint right after it reads its batch.supervisor could not be found). The task fails withCheckpoint request ... failed, dying, and nothing is published.The spec also used the default
startDelay(PT5S) andperiod(PT30S). The supervisor adds a newly created task to its task group only on its next run. Until then, a checkpoint request does nothing (New checkpoint is null) and leaves the task paused. The test depended on the suspend for publishing: the suspended supervisor's first run, 5 seconds later, set the end offsets.The fix suggested in the issue (wait for
ingest/events/processed>= 4 before suspending) makes the race less likely but does not remove it.processedis incremented during parsing, a few milliseconds before the checkpoint request is sent, and the metric is emitted every 100 ms.Fix
newKafkaSupervisor(), like the other tests in the class, with only the header filter added. The removed builder was identical apart from the timing settings: 500 ms task duration, 500 ms run period, and 10 ms start delay.@AfterEachteardown then suspends the supervisor and cancels the remaining tasks.Suspending a supervisor while a task's checkpoint is in flight fails that task. The failure is recoverable: the task's offsets were not committed, so resumed tasks re-read the data. This PR does not change that behavior.
Testing
Ran locally on JDK 25 with
-Dsurefire.rerunFailingTestsCount=0:With this change, handoff completes about 1.6 seconds after the supervisor starts, compared with about 5.5 seconds after the suspend before.
Key changed/added classes in this PR
EmbeddedKafkaSupervisorTestThis PR has: